Skip to content

fix(akamai): recover fetcher key on reconciled deploy status - #3193

Open
adityamisra08 wants to merge 5 commits into
mainfrom
fix/akamai-recover-fetcher-key
Open

fix(akamai): recover fetcher key on reconciled deploy status#3193
adityamisra08 wants to merge 5 commits into
mainfrom
fix/akamai-recover-fetcher-key

Conversation

@adityamisra08

Copy link
Copy Markdown
Contributor

Summary

  • deploy-status now returns the managed rule's fetcher key when deployed && freshWrite !== false — i.e. only when the key found belongs to THIS deploy, never a stale key inherited from a prior onboarding.
  • Lets the Akamai onboarding wizard recover the Bot Manager allowlist key after a deploy whose own HTTP response was lost to the CDN timeout, instead of forcing the customer to re-run Deploy (and rotate the key again) just to see a value Akamai already has.
  • Updated the doc comments on getManagedFetcherKey and deployStatus that previously asserted the key is "never returned" — narrowed to the new, guarded contract.
  • Updated the 3 existing deployStatus/freshWrite unit tests whose old assertions ("key is never returned") directly contradicted this intended behavior change.

Companion frontend PR: adobe/project-elmo-ui (same branch name fix/akamai-recover-fetcher-key).

Test plan

  • npx eslint clean on all 3 changed files
  • npx mocha test/controllers/llmo/llmo-akamai.test.js — 115/115 passing
  • npm run docs:lint — no new issues (pre-existing warnings only)
  • tsc type-check (via pre-commit hook) — passing

deploy-status now returns the managed rule's fetcher key, gated on
freshWrite !== false so a stale/inherited key from a prior onboarding
is never surfaced. Lets the onboarding UI recover the key for Bot
Manager allowlisting after a deploy response was lost to the CDN
timeout, instead of forcing a re-deploy just to see it again.
@codecov

codecov Bot commented Sep 3, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@MysticatBot MysticatBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @adityamisra08,

⚠ Degraded review - no spec document was found for this change (searched the PR links, the touched repos' docs, the architecture/guidelines docs, and linked Jira). This review covers code-level quality but could not validate the change against an agreed design, so confidence is reduced. Add a spec link (PR template section 4) and re-request review for a full-confidence pass.

Verdict: Request changes - one blocking finding on the API contract for fetcherKey: null.
Complexity: MEDIUM - small diff; API surface.
Changes: Returns the Akamai Bot Manager fetcher key in deploy-status responses when the key belongs to the current deploy, enabling recovery after CDN timeouts (3 files).
Note: CI checks are currently pending.

Must fix before merge

  1. [Important] fetcherKey: null returned when managed rule exists but fetcher-key header is absent - src/controllers/llmo/llmo-akamai.js:922 (details inline)
Non-blocking (2): minor issues and suggestions
  • nit: getManagedFetcherKey(ruleTree) is called twice on the same immutable tree when baseVersion is supplied; targetKey from the freshWrite comparison already holds the result - src/controllers/llmo/llmo-akamai.js:922
  • suggestion: add expect(body).to.not.have.property('fetcherKey') to the existing deployed:false test to lock in the new contract - test/controllers/llmo/llmo-akamai.test.js:559

Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 0m 54s | Cost: $6.66 | Commit: 91cba0fe92d6bcccbc05cdf7de1fe8ede170d1fd
If this code review was useful, please react with 👍. Otherwise, react with 👎.

Comment thread src/controllers/llmo/llmo-akamai.js
@MysticatBot MysticatBot added ai-reviewed Reviewed by AI complexity:medium AI-assessed PR complexity: MEDIUM labels Sep 3, 2026
Guard the recovered key against null (rule present but the
fetcher-key header itself is missing/malformed) so the response
omits the field instead of sending fetcherKey: null, which would
read as "recovered" to a consumer checking presence. Also hoists
the tree read so it's done once instead of twice.

Addresses review feedback on #3193.
@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown

This PR will trigger a patch release when merged.

@adityamisra08

Copy link
Copy Markdown
Contributor Author

All 3 items from the review addressed in f56b267:

  1. BlockingfetcherKey: null when the rule is present but the header itself is missing/malformed: fixed via the suggested recoveredKey guard (recoveredKey && freshWrite !== false).
  2. Nit — duplicate getManagedFetcherKey(ruleTree) call: fixed by hoisting into recoveredKey, reused for the freshWrite compare.
  3. Nit — lock in deployed:false contract with an explicit assertion: added, plus the same assertion on the deployed:true/no-header case (which is the actual null-guard regression test).

115/115 unit tests passing.

@MysticatBot MysticatBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @adityamisra08,

⚠ Degraded review - no spec document was found for this change (searched the PR links, the touched repos' docs, the architecture/guidelines docs, and linked Jira). This review covers code-level quality but could not validate the change against an agreed design, so confidence is reduced. Add a spec link (PR template section 4) and re-request review for a full-confidence pass.

Verdict: Approve - all prior findings addressed; no new blocking issues.
Complexity: MEDIUM - small diff; API surface.
Changes: Returns the Akamai Bot Manager fetcher key in deploy-status responses when the key belongs to the current deploy, enabling recovery after CDN timeouts (3 files).
Note: CI checks are currently pending.

Non-blocking (2): minor issues and suggestions
  • suggestion: The no-baseVersion path returns the fetcher key without freshWrite confirmation. The trust model is sound (callers already own the property's EdgeGrid credentials), but the JSDoc claim that the key is "confirmed to belong to THIS deploy" is slightly imprecise for this case - consider a brief note in the comment acknowledging the trust assumption - src/controllers/llmo/llmo-akamai.js:928
  • suggestion: Verify that the response's Cache-Control header (or Fastly's Surrogate-Control) prevents caching of this route, since deploy-status now carries a secret value

Previously flagged, now resolved

  • fetcherKey: null leak when managed rule exists but fetcher-key header is absent - now guarded by recoveredKey && freshWrite !== false
  • Duplicate getManagedFetcherKey(ruleTree) tree walk - hoisted into recoveredKey and reused for both the freshWrite compare and the response
  • Missing negative assertions on deployed:false and deployed:true/no-header tests - added, plus positive assertions on the recovery paths

Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 2m 14s | Cost: $5.08 | Commit: f56b26754e3a10744f71fdc3cc5aa52157ee8820
If this code review was useful, please react with 👍. Otherwise, react with 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai-reviewed Reviewed by AI complexity:medium AI-assessed PR complexity: MEDIUM

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants